Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 58.52%. Comparing base (b7e0b10) to head (52ebaf5).
Report is 337 commits behind head on master.

@@            Coverage Diff             @@
##           master    #8547      +/-   ##
==========================================
+ Coverage   58.02%   58.52%   +0.50%     
==========================================
  Files         388      388              
  Lines       38977    38958      -19     
==========================================
+ Hits        22617    22802     +185     
+ Misses      16360    16156     -204     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@t-brandt Looks good to me; can you please add a change log entry?
@hbushouse Can you please kick off a regression test run for this?

Ok, I updated CHANGES.rst.

Thanks- minor comment that the number of dashes underneath the title 'nsclean' should be the same number of characters as the step name. I'm not honestly sure why, but @hbushouse has frequently had to remind me about it.

Thanks- minor comment that the number of dashes underneath the title 'nsclean' should be the same number of characters as the step name. I'm not honestly sure why, but @hbushouse has frequently had to remind me about it.

The number of underline characters needs to be at least as long as the title above it, but can be longer (without doing harm). It just can't be shorter or the sphinx parser will barf.

Ok, I adjusted the number of dashes.

I tried to run regtests for this PR:
https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FJWST-Developers-Pull-Requests/detail/JWST-Developers-Pull-Requests/1513/tests
Unfortunately there are many errors that look unrelated (mostly in fgs related tests) but I'm not sure what's causing them. Here's a snippet from the traceback:

        # Make the query
        params = {'pattern': pattern}
        with requests.get(search_url, params=params, headers=headers) as r:
>           url_paths = r.json()['files']
E           KeyError: 'files'
/data1/jenkins/workspace/RT/JWST-Developers-Pull-Requests/clone/jwst/regtest/regtestdata.py:500: KeyError

It looks like these changes are only in the NSClean class. Is it possible to make similar changes in the NSCleanSubarray class too? I'm wondering if this would enable support for the ALLSLITS subarray - right now, it uses so much memory that it crashes the kernel if attempted, so support for this subarray is explicitly turned off.

@melanieclarke The NSCleanSubarray already does something close to the changes I made. Unfortunately, I think that the reason it fails is

This problem does not affect the full array version of NSClean because that operates row-by-row, so that there are never more than 2048 pixels to be fit simultaneously, meaning that the equivalent matrix is at most 2048^2:

This array size/memory issue in NSCleanSubarray can be fixed, but not quite so simply as with the pull request here. If it is a priority for the mission I can spend a bit more time to propose a fix.

Maybe I'm missing something, but can we simply swap subarray mode to run row-by-row as well?

@drlaw1558 Yes, we can do that. The best thing is probably to do something intermediate: to process as many rows simultaneously as we think we can profitably fit the Fourier modes. Assuming I am interpreting Bernie's code right, line

@t-brandt Sounds like that's sufficiently different that it makes sense to consider separately rather than tying it to the work currently here. I'll file a separate ticket to track it; https://jira.stsci.edu/browse/JP-3654

Another regtest run started at https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1524

Regtest run https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1525/ had no failures or differences, so this looks good.